Skip to content

OCPBUGS-27496#Update provisioner from in-tree to CSI #71398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPBUGS-27496#Update provisioner from in-tree to CSI #71398

wants to merge 1 commit into from

Conversation

lpettyjo
Copy link
Contributor

@lpettyjo lpettyjo commented Feb 8, 2024 •
edited
Loading

Version(s): 4.14+

Issue: https://issues.redhat.com/browse/OCPBUGS-27496

Link to docs preview: https://71398--ocpdocs-pr.netlify.app/openshift-enterprise/latest/storage/dynamic-provisioning

QE review:

  • QE has approved this change.

Additional information:

PTAL: @gcharot @jsafrane @duanwei33

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 8, 2024
@ocpdocs-previewbot
Copy link

🤖 Thu Feb 08 20:30:05 - Prow CI generated the docs preview: https://71398--ocpdocs-pr.netlify.app

@lpettyjo lpettyjo changed the title OCPBUGS-27496#Update provisioner from in-tree to CSI in dynamic provi… OCPBUGS-27496#Update provisioner from in-tree to CSI Feb 9, 2024
@lpettyjo lpettyjo added branch/enterprise-4.14 branch/enterprise-4.15 peer-review-needed Signifies that the peer review team needs to review this PR labels Feb 9, 2024
@lpettyjo lpettyjo added this to the Continuous Release milestone Feb 9, 2024
@skopacz1
Copy link
Contributor

skopacz1 commented Feb 9, 2024

/label peer-review-in-progress

@openshift-ci openshift-ci bot added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Feb 9, 2024
skopacz1
skopacz1 approved these changes Feb 9, 2024
View reviewed changes
Copy link
Contributor

@skopacz1 skopacz1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Good catch with the typo.

@skopacz1
Copy link
Contributor

skopacz1 commented Feb 9, 2024

/remove-label peer-review-needed
/remove-label peer-review-in-progress

/label peer-review-done

@openshift-ci openshift-ci bot added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Feb 9, 2024
jsafrane
jsafrane reviewed Feb 12, 2024
View reviewed changes
modules/dynamic-provisioning-aws-definition.adoc
@@ -13,7 +13,7 @@ kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
name: <storage-class-name> <1>
provisioner: kubernetes.io/aws-ebs
provisioner: ebs.csi.aws.com
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSI drivers can have different parameters than corresponding in-tree volume plugins.
For AWS EBS, 4.14 understands these options: https://github.com/openshift/aws-ebs-csi-driver/blob/release-4.14/docs/parameters.md (replace 4.14 in the URL with a different versions, 4.15 got a few new ones)

It seems that our documentation is still valid for AWS EBS. The CSI driver has more options that we don't document yet. And the question is if we should, because it's a huge effort - we would need to test the options first.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsafrane
How about just listing the OCP pre-defined storageclass content (like https://github.com/openshift/aws-ebs-csi-driver-operator/blob/master/assets/storageclass_gp3.yaml) in this "dynamic-provisioning" section and documenting more parameters when introducing the CSI Driver section?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work for me

modules/dynamic-provisioning-azure-disk-definition.adoc
@@ -13,7 +13,7 @@ apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
name: <storage-class-name> <1>
provisioner: kubernetes.io/azure-disk
provisioner: disk.csi.azure.com
volumeBindingMode: WaitForFirstConsumer <2>
allowVolumeExpansion: true
parameters:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Azure Disk CSI driver has their parameters documented here: https://github.com/openshift/azure-disk-csi-driver/blob/release-4.14/docs/driver-parameters.md

storageaccounttype is deprecated, skuName should be used instead. And there is a plenty of new options. Again, not tested by us, so question is, if they should be documented.

modules/dynamic-provisioning-azure-file-definition.adoc
@@ -45,7 +45,7 @@ kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
name: <azure-file> <1>
provisioner: kubernetes.io/azure-file
provisioner: file.csi.azure.com
parameters:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream docs: https://github.com/openshift/azure-file-csi-driver/blob/release-4.14/docs/driver-parameters.md

Our doc looks OK, upstream only documents much more options.

modules/dynamic-provisioning-cinder-definition.adoc
@@ -13,7 +13,7 @@ kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
name: <storage-class-name> <1>
provisioner: kubernetes.io/cinder
provisioner: cinder.csi.openstack.org
parameters:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream doc: https://github.com/openshift/cloud-provider-openstack/blob/release-4.14/docs/cinder-csi-plugin/using-cinder-csi-plugin.md#supported-parameters

Our doc looks OK.

modules/dynamic-provisioning-gce-definition.adoc
@@ -13,7 +13,7 @@ apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
name: <storage-class-name> <1>
provisioner: kubernetes.io/gce-pd
provisioner: pd.csi.storage.gke.io
parameters:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream doc: https://github.com/openshift/gcp-pd-csi-driver/blob/release-4.14/README.md#createvolume-parameters

Our doc looks OK, upstream only documents has few more options

@kalexand-rh
Copy link
Contributor

The branch/enterprise-4.16 label has been added to this PR.

This is because your PR targets the main branch and is labeled for enterprise-4.15. And any PR going into main must also target the latest version branch (enterprise-4.16).

If the update in your PR does NOT apply to version 4.16 onward, please re-target this PR to go directly into the appropriate version branch or branches (enterprise-4.x) instead of main.

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 26, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2024
@openshift-merge-robot
Copy link

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 1, 2024
@openshift-bot
Copy link

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci openshift-ci bot closed this Dec 1, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Dec 1, 2024

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@lpettyjo
Copy link
Contributor Author

lpettyjo commented Dec 2, 2024

/remove-lifecycle rotten

@openshift-ci openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Dec 2, 2024
@lpettyjo lpettyjo reopened this Dec 2, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Dec 2, 2024

@lpettyjo: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/validate-portal f2e98a8 link true /test validate-portal
ci/prow/validate-asciidoc f2e98a8 link true /test validate-asciidoc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@bergerhoffer
Copy link
Contributor

The branch/enterprise-4.19 label has been added to this PR.

This is because your PR targets the main branch and is labeled for enterprise-4.18. And any PR going into main must also target the latest version branch (enterprise-4.19).

If the update in your PR does NOT apply to version 4.19 onward, please re-target this PR to go directly into the appropriate version branch or branches (enterprise-4.x) instead of main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsafrane jsafrane jsafrane left review comments

@duanwei33 duanwei33 duanwei33 left review comments

@skopacz1 skopacz1 skopacz1 approved these changes

Assignees
No one assigned
Labels
branch/enterprise-4.14 branch/enterprise-4.15 branch/enterprise-4.16 branch/enterprise-4.17 branch/enterprise-4.18 branch/enterprise-4.19 needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
Continuous Release
Development

Successfully merging this pull request may close these issues.
9 participants
@lpettyjo @ocpdocs-previewbot @skopacz1 @kalexand-rh @openshift-bot @openshift-merge-robot @bergerhoffer @jsafrane @duanwei33